Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flow ILogger to InstrumentationHelper 2 #727

Merged
merged 31 commits into from
Mar 13, 2020

Conversation

daveMueller
Copy link
Collaborator

closes #559

@daveMueller
Copy link
Collaborator Author

Mhh when running the tests locally on my machine they pass.

image

@daveMueller
Copy link
Collaborator Author

I'm on vacation for a week and will check this when I'm back. If someone wants to take over please feel free to do so.

@daveMueller daveMueller mentioned this pull request Feb 10, 2020
6 tasks
@MarcoRossignoli MarcoRossignoli added the enhancement General enhancement request label Feb 10, 2020
@MarcoRossignoli
Copy link
Collaborator

@daveMueller thank's and have a nice holiday!

@daveMueller
Copy link
Collaborator Author

Mhh when running the tests locally on my machine they pass.

image

For whatever reason the LazyInitializer.EnsureInitialized initialized a new container for the two tests. The tests only failed if another test that initialized the container was run before them. When I ran the tests isolated they passed as seen in the screenshot.

@MarcoRossignoli
Copy link
Collaborator

@daveMueller can you rebase you branch?There were some error on that tests...now they run out of process maybe fix your issue, try to use the ensure initializer #735

@daveMueller
Copy link
Collaborator Author

Yes sorry if it wasn't clear. It is ready to review.

@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Mar 1, 2020

During review I had some doubt on msbuild tasks.
With this update we should remove also InitDefaultServices() from DependencyInjection.cs, when we do it we MUST be sure that CoverageResultTask will be hosted in the same process as InstrumentationTask and also that Log instance passed to InstrumentationTask is the same of CoverageResultTask otherwise we could get exception if the task log instance used by IInstrumentationHelper(if first Log instance is disposed by msbuild engine).

We're sure with .net tool(console) and collector(datacollector process) but I'm not sure about one or more msbuild Task. We need to understand well if when we inject task in msbuild through https://github.com/tonerdo/coverlet/blob/master/src/coverlet.msbuild.tasks/coverlet.msbuild.targets they run in same process, I think so because otherwise also restore dll https://github.com/tonerdo/coverlet/blob/master/src/coverlet.core/Helpers/InstrumentationHelper.cs#L24 could race with possible CoverageResultTask loaded in another process.

What do you think?

@MarcoRossignoli MarcoRossignoli added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 1, 2020
@daveMueller
Copy link
Collaborator Author

Yes I agree with removing InitDefaultServices().

We need to understand well if when we inject task in msbuild through https://github.com/tonerdo/coverlet/blob/master/src/coverlet.msbuild.tasks/coverlet.msbuild.targets they run in same process,

That's what I would expect but I'm also not sure. I try to do some research here.

@daveMueller
Copy link
Collaborator Author

daveMueller commented Mar 7, 2020

According to the question I raised here dotnet/msbuild#5153 we shouldn't have a problem as long as the tasks are only used in the coverlet.msbuild.tasks project.
When I was thinking about this the last couple of days I also don't really see the advantage of separating this into two different tasks. E.g. the coverlet.console project is also doing both, instrumenting and computing the results in the Program.cs class.

@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Mar 8, 2020

Thanks a lot for the investigation on msbuild repo, great work Dave!

It's separated from the beginning to be usable in different stages(before buildproject/vstest and after) orchestrated by msbuild.
Btw seems ok from DI perspective, but that sentence on Log is problematic, we need to pass log out of container because we register only the one of instrument task.

@MarcoRossignoli
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MarcoRossignoli MarcoRossignoli removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 13, 2020
@MarcoRossignoli MarcoRossignoli merged commit 34d6dc5 into coverlet-coverage:master Mar 13, 2020
@daveMueller daveMueller deleted the 559_FlowILogger2 branch October 27, 2023 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General enhancement request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flow ILogger to InstrumentationHelper
2 participants